QOL make our gem requireable and autoload/eager load some constants #171

davebenvenuti · 2025-01-15T00:17:45Z

Pairing with @nirvdrum , we noticed require "protoboeuf" didn't work despite having it in our Gemfile which led to some confusion (#170). This makes require "protoboeuf" work and auto- and eagerloads some constants for the user.

ProtoBoeuf::CodeGen is now autoloaded from the base lib/protoboeuf.rb
ProtoBoeuf::Google is also now autoloaded from the base lib/protoboeuf.rb
Everything in lib/protoboeuf/google/**/*.rb is eager loaded when ProtoBoeuf::Google loads. There isn't a clean 1:1 mapping between constants and *.rb files so autoload would be fairly tricky. Although this could be a controversial decision, I think this makes things easier to work with. But feedback is welcome.
rake well_known_types now generates modules that autoload all child constants for well known types (eg: ProtoBoeuf::Google::Protobuf with the help of ProtoBoeuf::AutoloaderGen!

rwstauner

Makes sense, just a few small things to fix

test/gem_test.rb

lib/protoboeuf/google.rb

tenderworks · 2025-01-16T01:18:59Z

lib/protoboeuf/google.rb

+
+# There isn't a clean 1:1 mapping between constants and *.rb files, so eager load instead of autoload.
+
+Dir[File.expand_path("google/**/*.rb", __dir__)].each { |file| require file }


I'd prefer we don't do this pattern, and rather generate the requires as part of our .proto to .rb step.

2 reasons:

It's important to know what we're requiring

I'm not sure if directory listings are sorted (they weren't in the past) and it can cause platform dependent problems.

If generating a file with the requires is to onerous, I can understand and we can go this route, but I'd prefer if we didn't.

I'll give that a whirl

Added a new commit: 7dab057

We now generate modules in lib/protoboeuf/google/api.rb and lib/protoboeuf/google/protobuf.rb that autoload all of the constants defined in lib/protoboeuf/google/**/*.rb! Note that they're collapsed by default in the PR diff view because I marked them as autogenerated in .gitattributes. Big thanks to @rwstauner for helping me update the Rakefile in a way where I wouldn't embarrass myself.

https://github.com/Shopify/protoboeuf/blob/7dab057840f374538c061ce438f3b1db5a41f739/lib/protoboeuf/google/api.rb

https://github.com/Shopify/protoboeuf/blob/7dab057840f374538c061ce438f3b1db5a41f739/lib/protoboeuf/google/protobuf.rb

…r generated constants

tenderworks

This is great, thank you!

rwstauner · 2025-01-22T00:24:53Z

lib/protoboeuf/autoloadergen.rb

+      @parent_module_parts = parent_module.split("::")
+
+      # Given lib/protoboeuf/google.rb, glob lib/protoboeuf/google/**/*.rb
+      @child_ruby_filenames = Dir[module_filename.pathmap("%X/**/*.rb")].sort


This String#pathmap comes from rake.
Can we simplify that to get rid of that dependency?
Maybe just Dir["#{module_filename.delete_suffix(".rb")}/**/*.rb"] would be enough?

lib/protoboeuf/autoloadergen.rb

rwstauner · 2025-01-22T00:30:03Z

lib/protoboeuf/autoloadergen.rb

+        # For the autoloader_full_module_name we can just pick the first child constant we come across and take the
+        # first three parts.  For example, ProtoBoeuf::Google::Api::FieldBehavior would be ProtoBoeuf::Google::Api.
+        if @autoloader_module_name.nil?
+          autoloader_full_module_name = child_constants.first.split("::")[0..2].join("::")


Can we generalize this 2 in case there's ever a different level of nesting?
What do we really want here, everything but the last one?
or one more than whatever is in parent_module?

I turned this into a sub w/ regex based on parent_module.

Thanks, that does make the intention clearer.

rwstauner · 2025-01-22T00:30:50Z

lib/protoboeuf/autoloadergen.rb

+        end
+      end
+
+      @generated_autoloader_module_parts = autoloader_full_module_name.split("::")


Do we use this value for something else, or are we joining it just to split it again?
Is there a better way to store this?

We use it in #to_ruby to define the nested module blocks, so an array (or some kind of iterable collection) is probably the best way to store it.

<%- generated_autoloader_module_parts.each do |module_name| -%> module <%= module_name %> <%- end -%>

Sorry, what I was referring to is that when we set the autoloader_full_module_name local var we join it, and the only thing we do with it after that is split it. We could skip both of those calls.

rwstauner

Looks good, just a few questions.

Rakefile

Co-authored-by: Randy Stauner <[email protected]>

davebenvenuti requested review from nirvdrum, rwstauner and tenderworks January 15, 2025 16:40

rwstauner requested changes Jan 15, 2025

View reviewed changes

test/gem_test.rb Outdated Show resolved Hide resolved

lib/protoboeuf/google.rb Outdated Show resolved Hide resolved

davebenvenuti added 2 commits January 15, 2025 12:34

Make this gem requireable

ff77682

Auto/eager load constants for convenience

ecdc7b5

davebenvenuti force-pushed the davebenvenuti/make-require-protoboeuf-work branch from cafa5bd to ecdc7b5 Compare January 15, 2025 20:34

davebenvenuti requested a review from rwstauner January 15, 2025 23:54

rwstauner approved these changes Jan 16, 2025

View reviewed changes

tenderworks reviewed Jan 16, 2025

View reviewed changes

davebenvenuti marked this pull request as draft January 17, 2025 00:07

davebenvenuti force-pushed the davebenvenuti/make-require-protoboeuf-work branch 8 times, most recently from 5e8c636 to a4ea5b8 Compare January 17, 2025 21:28

ProtoBoeuf::AutoloaderGen will generate helper modules to autoload ou…

7dab057

…r generated constants

davebenvenuti force-pushed the davebenvenuti/make-require-protoboeuf-work branch from a4ea5b8 to 7dab057 Compare January 17, 2025 21:30

davebenvenuti marked this pull request as ready for review January 17, 2025 21:35

davebenvenuti requested review from rwstauner and tenderworks January 17, 2025 21:35

tenderworks approved these changes Jan 21, 2025

View reviewed changes

rwstauner reviewed Jan 22, 2025

View reviewed changes

rwstauner approved these changes Jan 22, 2025

View reviewed changes

Rakefile Outdated Show resolved Hide resolved

davebenvenuti and others added 2 commits January 22, 2025 09:16

Using a regex will generalize this a bit more

e39361d

Simplify pathmap

260c5d0

Co-authored-by: Randy Stauner <[email protected]>

davebenvenuti merged commit d51fe05 into main Jan 22, 2025
7 checks passed

davebenvenuti deleted the davebenvenuti/make-require-protoboeuf-work branch January 22, 2025 17:47

davebenvenuti added the #gsd:43107 Ruby Language: RPC Code Generation for Shopify Internal APIs [GSD 43107] label Jan 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QOL make our gem requireable and autoload/eager load some constants #171

QOL make our gem requireable and autoload/eager load some constants #171

davebenvenuti commented Jan 15, 2025 •

edited

Loading

rwstauner left a comment

tenderworks Jan 16, 2025

davebenvenuti Jan 16, 2025

davebenvenuti Jan 17, 2025 •

edited

Loading

tenderworks left a comment

rwstauner Jan 22, 2025

rwstauner Jan 22, 2025

davebenvenuti Jan 22, 2025

rwstauner Jan 22, 2025

rwstauner Jan 22, 2025

davebenvenuti Jan 22, 2025 •

edited

Loading

rwstauner Jan 22, 2025

rwstauner left a comment


		# There isn't a clean 1:1 mapping between constants and *.rb files, so eager load instead of autoload.

		Dir[File.expand_path("google/*/.rb", __dir__)].each { \|file\| require file }

QOL make our gem requireable and autoload/eager load some constants #171

QOL make our gem requireable and autoload/eager load some constants #171

Conversation

davebenvenuti commented Jan 15, 2025 • edited Loading

rwstauner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davebenvenuti Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

tenderworks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davebenvenuti Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwstauner left a comment

Choose a reason for hiding this comment

davebenvenuti commented Jan 15, 2025 •

edited

Loading

davebenvenuti Jan 17, 2025 •

edited

Loading

davebenvenuti Jan 22, 2025 •

edited

Loading